Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes issue #194 related to generated columns that reference functions from different schemas. The fix ensures that cross-schema function references in generated column expressions are properly qualified with their schema names.
Key Changes:
- Modified the
GetColumnsForSchemaSQL query to use a LATERAL join withset_configto control thesearch_pathwhen evaluating generated column expressions - Added test data demonstrating a generated column that calls a function from a different schema (
util.extract_domain) - Improved parameter naming from
dollar_1totableSchemain the Go code
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
ir/queries/queries.sql |
Refactored GetColumnsForSchema query with CTE and LATERAL join to properly qualify cross-schema function references in generated columns |
ir/queries/queries.sql.go |
Generated Go code reflecting the SQL query changes and improved parameter naming |
testdata/dump/tenant/util.sql |
Added extract_domain function to test cross-schema function usage |
testdata/dump/tenant/tenant.sql |
Added website column and domain generated column to users table for testing |
testdata/dump/tenant/pgschema.sql |
Expected output showing properly qualified function reference in generated column |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ge.generated_expr | ||
| FROM column_base cb | ||
| LEFT JOIN LATERAL ( | ||
| SELECT |
There was a problem hiding this comment.
The dummy column in the LATERAL subquery is used solely to ensure set_config is evaluated before pg_get_expr. While this works, it's a workaround that relies on left-to-right evaluation order in the SELECT clause. Consider adding a comment explaining this pattern or using a more explicit approach, such as a custom function that wraps pg_get_expr with search_path control.
| SELECT | |
| SELECT | |
| -- The dummy column ensures set_config is evaluated (for its side effect) | |
| -- before pg_get_expr, relying on PostgreSQL's left-to-right SELECT evaluation. |
| FROM column_base cb | ||
| LEFT JOIN LATERAL ( | ||
| SELECT | ||
| set_config('search_path', cb.table_schema || ', pg_catalog', true) as dummy, |
There was a problem hiding this comment.
The search_path is constructed using string concatenation: cb.table_schema || ', pg_catalog'. While schema names from information_schema.columns are generally safe, PostgreSQL allows schema names with special characters, quotes, or commas. If a schema name contains a comma or other special characters, this could lead to unexpected behavior or potentially a security issue. Consider using proper quoting for the schema name, such as using quote_ident() or validating that the schema name doesn't contain problematic characters.
| set_config('search_path', cb.table_schema || ', pg_catalog', true) as dummy, | |
| set_config('search_path', format('%I, pg_catalog', cb.table_schema), true) as dummy, |
| FROM column_base cb | ||
| LEFT JOIN LATERAL ( | ||
| SELECT | ||
| set_config('search_path', cb.table_schema || ', pg_catalog', true) as dummy, |
There was a problem hiding this comment.
The search_path is constructed using string concatenation: cb.table_schema || ', pg_catalog'. While schema names from information_schema.columns are generally safe, PostgreSQL allows schema names with special characters, quotes, or commas. If a schema name contains a comma or other special characters, this could lead to unexpected behavior or potentially a security issue. Consider using proper quoting for the schema name, such as using quote_ident() or validating that the schema name doesn't contain problematic characters.
| set_config('search_path', cb.table_schema || ', pg_catalog', true) as dummy, | |
| set_config('search_path', quote_ident(cb.table_schema) || ', pg_catalog', true) as dummy, |
| func (q *Queries) GetColumnsForSchema(ctx context.Context, tableSchema sql.NullString) ([]GetColumnsForSchemaRow, error) { | ||
| rows, err := q.db.QueryContext(ctx, getColumnsForSchema, tableSchema) |
There was a problem hiding this comment.
The parameter name tableSchema is more descriptive than dollar_1, but this creates inconsistency with other similar functions in this file (e.g., GetAggregatesForSchema, GetConstraintsForSchema, etc.) which still use dollar_1. Consider updating all similar functions to use descriptive parameter names for consistency.
| func (q *Queries) GetColumnsForSchema(ctx context.Context, tableSchema sql.NullString) ([]GetColumnsForSchemaRow, error) { | |
| rows, err := q.db.QueryContext(ctx, getColumnsForSchema, tableSchema) | |
| func (q *Queries) GetColumnsForSchema(ctx context.Context, dollar_1 sql.NullString) ([]GetColumnsForSchemaRow, error) { | |
| rows, err := q.db.QueryContext(ctx, getColumnsForSchema, dollar_1) |
tianzhou
force-pushed
the
public_schema_generarated_column
branch
from
661747c to
0ebbde8
Compare
1 participant
Fix #194